chore(deps): reduce strictness of dependencies #41
Conversation
setup.py
Outdated
| license="MIT", | ||
| packages=["python_graphql_client"], | ||
| install_requires=["aiohttp==3.7.3", "requests==2.22.0", "websockets==8.1"], | ||
| install_requires=["aiohttp>=3.0.0", "requests>=1.0.0", "websockets>=5.0"], |
There was a problem hiding this comment.
This looks good but I'm a bit concerned that these libraries release a new major version with lots of breaking changes since that's what major versions are for. So, I was just wondering what would be the pip equivalent of the caret(^) range of npm. I guess we can do something like aiohttp==3.*.* to limit the major version?
There was a problem hiding this comment.
For sure, we can definitely do that! I think the ~= is what would work here.
| install_requires=["aiohttp>=3.0.0", "requests>=1.0.0", "websockets>=5.0"], | |
| install_requires=["aiohttp~=3.0", "requests~=2.0", "websockets>=5.0,<9"], |
I initially went with a looser restriction on versions since I wasn't sure if it was better to be preventative or reactive to issues caused by major version changes of dependencies. If we restrict to a major version here, say requests v2, then we will need to actively monitor and respond to major version changes of that project. Really the tradeoff is whether we want to avoid any dependency issues and manually test a version bump of a dependency when it comes out or if we want to react to issues as people identify them with allowed versions of dependencies. I think there's merit to both approaches, and the issues I see with manually checking for dependency version bumps is less of a factor if we have something like dependabot or another similar service giving us alerts when a new major version comes out.
There was a problem hiding this comment.
Yeah, it sounds like you have great insight into the trade-offs. Leveraging tools like dependabot or renovate for automating dependency management sounds like a good idea. 👍
tests/test_graphql_client.py
Outdated
| json={"query": query}, | ||
| headers={}, | ||
| auth=HTTPBasicAuth("[email protected]", "not_a_real_password"), | ||
| auth=auth, |
There was a problem hiding this comment.
Thanks for fixing this but how about we drop support for request v1? I feel like that would be more generous than needed because it's never been supported since this library was created.
There was a problem hiding this comment.
The last v1 release of requests came out in 2013, so I think that's fair. I was trying to super flexible here with what versions we allow, but in reality this is probably unecessary.
There was a problem hiding this comment.
I don't feel strongly about this. I'll leave it up to you. 😃
setup.py
Outdated
| license="MIT", | ||
| packages=["python_graphql_client"], | ||
| install_requires=["aiohttp==3.7.3", "requests==2.22.0", "websockets==8.1"], | ||
| install_requires=["aiohttp>=3.0.0", "requests>=1.0.0", "websockets>=5.0"], |
There was a problem hiding this comment.
Yeah, it sounds like you have great insight into the trade-offs. Leveraging tools like dependabot or renovate for automating dependency management sounds like a good idea. 👍
tests/test_graphql_client.py
Outdated
| json={"query": query}, | ||
| headers={}, | ||
| auth=HTTPBasicAuth("[email protected]", "not_a_real_password"), | ||
| auth=auth, |
There was a problem hiding this comment.
I don't feel strongly about this. I'll leave it up to you. 😃
|
@DaleSeo thanks for the review. For posterity's sake, I went with using the compatible release syntax to restrict it to the current major version of |
What kind of change does this PR introduce?
Not 100% what to classify this as, but I guess it falls under a feature (non-breaking change).
What is the current behavior?
As per #40, currently projects using this library have to use a very specific version of dependencies which this libraray prescribes. Instead of pinning the versions, we should be allowing all versions of our dependencies which don't break for this project.
What is the new behavior?
With the change here, the project has converted from using
==to>=for dependency versions. This still keeps some restrictions on anyone who uses this library to make sure the code functions properly, but also more flexibility in dpendency versions which are acceptable for this project.Does this PR introduce a breaking change?
No changes, anyone who uses this version of the libraray once it gets released won't have to pin their dependencies with previously limited versions.
Other information
I did find one bug when testing
requests==1.0.0which caused the tests to break. Specifically, in past versions of the requests library asserting equality onHTTPBasicAuthfailed because they were instantiated as two distinct instances of hte class. It seems that this was fixed later on so as the equality check would pass, albeit I'm not sure when this fix was made.Side note, if anyone is interested in how I automaated the process of checking which versions of the project worked / didn't work, let me know and I can put those steps in the comments on this PR.